Implement server-side ad slot templates with PBS and APS auction#680
Implement server-side ad slot templates with PBS and APS auction#680prk-Jr wants to merge 109 commits into
Conversation
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Incorporate all review feedback (aram356 + jevansnyc): cache contract, consent/GDPR gating, async restructuring detail, CreativeOpportunityFormat schema, glob pattern fix, XSS escaping, win notifications, APS params, timeout config key, defineSlot fix, gpt.rs ownership, KV migration path, Phase 2 sketch - Fix Prettier formatting (format-docs CI) - Add implementation plan (12 tasks, TDD, ordered by dependency)
- Incorporate all review feedback (aram356 + jevansnyc): cache contract, consent/GDPR gating, async restructuring detail, CreativeOpportunityFormat schema, glob pattern fix, XSS escaping, win notifications, APS params, timeout config key, defineSlot fix, gpt.rs ownership, KV migration path, Phase 2 sketch - Fix Prettier formatting (format-docs CI) - Add implementation plan (12 tasks, TDD, ordered by dependency)
Replace the head-injected __ts_bids design with a server-cached bid delivery model fetched by the client via a new /ts-bids endpoint. The auction never blocks page rendering — </head> flushes immediately, body parses without waiting for bids, and the client fetches bids in parallel with content paint. Key changes: - §2 Goal: bid delivery decoupled from page rendering; FCP unchanged from no-TS baseline - §4.3 Auction Trigger: drop buffered/streaming dichotomy; single mode forces chunked encoding on all origins (WordPress, NextJS, etc.) - §4.4 Head Injection: only __ts_ad_slots and __ts_request_id injected at <head> open; bid results moved to /ts-bids endpoint - §4.6 Client Residual: __tsAdInit defines slots immediately, fetches bids via /ts-bids, applies targeting and fires refresh() after resolve - §4.7 (new) Caching Behavior: explicit cacheability table for HTML, JS, CSS, tsjs bundle, bid results; Fastly edge HTTP cache leveraged for origin HTML - §5 Request-Time Sequence: full mermaid diagram covering content + creative + burl flow with cache-hit and cache-miss branches; separate text sequences for cache-hit (~80ms FCP, ~900ms ad-visible) and cache-miss (~250ms FCP, ~1,050ms ad-visible) - §6 Performance Summary: cache-hit and cache-miss columns; FCP added as a tracked metric - §7 Implementation Scope: add bid_cache.rs, /ts-bids endpoint, force chunked encoding step - §8 Edge Cases: origin-agnostic entries; new entries for /ts-bids 404 and client-never-fetches-/ts-bids Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pivot from the /ts-bids fetch endpoint + in-process bid_cache design to
inline __ts_bids injection before </body>. The earlier design relied on
shared state that doesn't reliably survive Fastly Compute's per-request Wasm
isolate model — body injection achieves the same FCP property in a single
response with no shared-state requirement.
Key changes:
- §4.3: replace /ts-bids long-poll with bounded </body> hold tied to
A_deadline. Body content above </body> paints first; close-tag held
until auction completes or A_deadline fires (graceful __ts_bids = {}
fallback).
- §4.3: add auction-eligibility gating (consent, bot UA, prefetch hints,
HEAD method, slot match) so auctions fire on real first-page-load
impressions only.
- §4.4: replace __ts_request_id + /ts-bids machinery with two inline
<script> blocks — __ts_ad_slots at <head> open, __ts_bids before
</body> via lol_html el.on_end_tag().
- §4.5: move both nurl and burl to client-side firing from
slotRenderEnded after hb_adid match. Server-side firing rejected to
avoid billing inflation on bids that never render.
- §4.6: replace fetch+Promise pattern with synchronous __ts_bids read.
Add lazy slim-Prebid loader (post-window.load) for scroll/refresh
auctions and Phase B identity warm-up. Add ts_initial=1 slot-ownership
sentinel.
- §4.7: switch Cache-Control from private, no-store to private,
max-age=0 to preserve browser BFCache eligibility while still
preventing intermediate-cache leaks.
- §4.8 (new): document the EC/KV identity model as load-bearing auction
input — Phase A retrieval at request time, Phase B post-render
enrichment via slim-Prebid userID modules. Add bare-EC first-impression
caveat and auction_eid_count metric. Note federated-consortium
passphrase property and clickstream-compounding speed win.
- §5: update mermaid + cache-hit/miss timelines for bounded body hold;
ad-visible converges to ~870ms (hit) / ~1,020ms (miss).
- §6: drop /ts-bids RTT row; add DCL row; add clickstream-compounding,
TS-overhead, identity-coverage, and confidence-interval framing.
- §7: drop bid_cache.rs and /ts-bids endpoint from scope; add
auction-eligibility gating and slim-Prebid bundle build target. Add
explicit "Deleted" subsection.
- §8: drop /ts-bids edge cases; add SPA/pushState, bare-EC, bot/prefetch,
HEAD, BFCache restoration cases.
- §9.6: server-side GAM downgraded from "Phase 2 commitment" to
aspirational and contingent on Google agreement. §9.8 (slim-Prebid
bundle composition), §9.9 (Privacy Sandbox), §9.10 (per-bidder consent)
added as follow-ups.
Implementation plan at docs/superpowers/plans/2026-04-30-server-side-ad-templates.md
is now stale relative to this spec; needs regenerating before code lands.
…ities.toml Adds the creative_opportunities field to Settings struct to deserialize configuration for the server-side ad auction feature. Includes build.rs stubs for types required during build-time configuration validation. Creates creative-opportunities.toml with example slot configuration and updates trusted-server.toml with the [creative_opportunities] section defining GAM network ID, auction timeout, and price granularity settings. Tests pass with proper TOML parsing of the creative_opportunities section.
…ared auction state
- Add `ad_slots_script: Option<String>` and `ad_bids_state: Arc<RwLock<Option<String>>>` fields to `HtmlProcessorConfig`
- Update `from_settings` to initialize both new fields with safe defaults
- Prepend `ad_slots_script` inside the existing `<head>` handler before integration inserts
- Add `element!("body", ...)` handler that uses `end_tag_handlers()` to inject `__ts_bids` before `</body>`; falls back to empty `{}` when auction state is `None`
- Add `IntegrationRegistry::empty_for_tests()` test helper
- Add three new tests covering all injection paths
…gibility gates; max-age=0 - Make handle_publisher_request async; add orchestrator and slots_file params - Dispatch origin request with send_async before running auction in parallel - Gate auction on GET, no prefetch, no bot, matched slots, TCF purpose-1 consent - Run server-side auction and write bucketed bids to ad_bids_state Arc<RwLock> - Compute ad_slots_script after response headers; set Cache-Control: private, max-age=0 - Fix Stream arm to thread actual ad_slots_script and ad_bids_state through - Add build_auction_request, build_bid_map, build_bids_script, build_ad_slots_script helpers - Update route_tests.rs to pass empty slots_file to route_request
…m slotRenderEnded
- build_bid_map now returns serde_json::Map with full bid objects (hb_pb,
hb_bidder, hb_adid, nurl, burl) instead of a plain CPM string map
- build_bids_script / build_ad_slots_script now emit full <script> tags
using JSON.parse("…") for safe inline embedding; add html_escape_for_script helper
- build_ad_slots_script uses correct property names (gam_unit_path, div_id,
formats, targeting) matching the client-side TSJS bundle expectations
- Replace map_or(false, …) with is_some_and(…) on lines 546, 549, 567
- Add # Panics doc sections to handle_publisher_request and create_html_processor
…nities.toml at startup
… from slotRenderEnded; slim-Prebid lazy loader
- Enable APS and adserver_mock in auction config; set providers and mediator - Increase auction_timeout_ms from 500ms to 3000ms — 500ms was too tight for HTTPS round-trips to mocktioneer, leaving the mediator zero budget - Fix mediation request: send numeric price instead of opaque encoded_price; mocktioneer requires a decoded price field and does not support encoded_price - Expand creative-opportunities slot page_patterns to include /news/**
Define SlotRenderEndedEvent, SlotRenderEvent, and TestWindow types to eliminate all @typescript-eslint/no-explicit-any violations in gpt/index.ts and gpt/index.test.ts. Extend GptWindow with __tsjs_slim_prebid_url so installSlimPrebidLoader avoids the any cast.
prk-Jr
changed the title
Set gam_network_id to 88059007 (autoblog production network). Update atf_sidebar_ad slot to /88059007/autoblog/news with div_id ad-atf_sidebar-0-_r_2_ (desktop ATF sidebar, 300x250); restrict page_patterns to article paths only (/20**, /news/**) since that div does not exist on the homepage. Add homepage_header_ad slot targeting /88059007/autoblog/homepage with ad-header-0-_R_jpalubtak5lb_ for 970x90/728x90/970x250 leaderboard formats. Reduce auction_timeout_ms from 3000 to 500 to cap TTFB at the spec-recommended ceiling.
Operator [response_headers] could reintroduce Surrogate-Control / Fastly-Surrogate-Control on responses the publisher path had marked private, making per-user HTML eligible for shared surrogate caching again. finalize_response now skips Cache-Control and both surrogate headers whenever the response already carries a private directive. GET /__ts/page-bids dispatched real PBS/APS auctions with no same-origin check, so any third-party page could trigger partner calls from a visitor's browser. The handler now requires Sec-Fetch-Site: same-origin/same-site, or the non-simple X-TSJS-Page-Bids header when fetch metadata is absent (legacy clients), and returns 403 otherwise. The tsjs SPA hook sends the header on every page-bids fetch.
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Automated review:
Review Summary
Reviewed PR #680 at 9613aff against origin/main, including current CI/review context and focused passes over Rust auction/publisher paths, GPT/Prebid JS, config/build validation, and caching/security behavior. CI is currently green, and many prior review findings have been fixed. I still found blocking correctness/compatibility issues in the server-side ad-template flow, especially around win/billing beacons and cache behavior.
Findings
P0 / Blockers
None.
P1 / High
-
Win and billing beacons can fire for non-Trusted-Server winners —
crates/js/lib/src/integrations/gpt/index.ts:463- Issue: The
slotRenderEndedhandler treats retained GPT slot targeting as proof that the TS bid won. PBS bids comparehb_adidagainstslot.getTargeting('hb_adid'), and APS bids only require!!bid.hb_bidder. Slot targeting is request state, not proof of the winning GAM line item; direct/sponsorship/house line items can render non-empty while TS targeting remains on the slot. - Why it matters:
nurl/burlare win/billing notifications. False positives can over-report wins and trigger billing for impressions won by other GAM demand. APS is especially exposed because the fallback has no event/creative identity check. - Suggested fix: Fire beacons only from a positive TS creative signal, such as a TS/Prebid creative callback carrying the expected ad/cache id, or a configured line-item/creative-id allowlist checked against
slotRenderEndedevent fields. Add a test where a non-empty non-TS line item renders while TS targeting remains on the slot.
- Issue: The
-
Ad-template cache policy is applied to every HTML response, even when the stack is disabled/no-op —
crates/trusted-server-core/src/publisher.rs:1219- Issue: All proxied
text/htmlresponses are rewritten toCache-Control: private, max-age=0and have surrogate headers stripped whenever this path handles HTML, regardless of whether any creative-opportunity slot matched or whether the checked-in/default config has zero slots. - Why it matters: Deploying with the current default/empty slot config can silently remove public/shared cacheability from all publisher HTML even though no per-user
tsjs.adSlots/tsjs.bidsdata was injected. That is a severe compatibility/performance regression and undermines the empty-slot kill-switch behavior. - Suggested fix: Only force private/no-surrogate for responses that actually need per-user protection (for example when matched slots/ad scripts/bids are injected, or another per-user response mutation requires it). Add tests for zero-slot and non-matching-slot configs preserving origin cache headers unless per-user data is injected.
- Issue: All proxied
P2 / Medium
-
/__ts/page-bidsaccepts same-site cross-origin callers —crates/trusted-server-core/src/publisher.rs:1580- Issue:
Sec-Fetch-Site: same-siteincludes sibling origins under the same registrable domain. This endpoint is side-effecting and can dispatch real PBS/APS auctions while forwarding request-derived signals to partners. - Why it matters: An untrusted same-site sibling origin cannot read the JSON without CORS, but it can still trigger partner calls and burn SSP quota from the visitor's browser context.
- Suggested fix: Require
Sec-Fetch-Site: same-originfor Fetch-Metadata-capable browsers. Keep the legacy fallback only whenSec-Fetch-Siteis absent and the non-simpleX-TSJS-Page-Bidsheader is present.
- Issue:
-
Repeated
requestBids()calls can drop inline server-side bidder params —crates/js/lib/src/integrations/prebid/index.ts:496- Issue: The shim mutates
unit.bidsin-place to keep onlytrustedServerand client-side bidders. On the nextrequestBids()call with the same Prebid ad unit object, the original server-side bidder entries are gone, sobidderParamsis rebuilt as{}and overwrites the existing trustedServer bidder params. - Why it matters: Publishers that supplied inline PBS params on the initial ad unit can lose them on refresh/re-auction and fall back to empty params/stored-request behavior instead.
- Suggested fix: Preserve original server-side bids in a side table/non-destructive clone, or retain existing
trustedServer.params.bidderParamswhen no new server-side bids are found. Add a test that callsrequestBids()twice with the same ad unit.
- Issue: The shim mutates
-
SPA auctions can apply before the new route's ad containers exist —
crates/js/lib/src/integrations/gpt/index.ts:548- Issue: After the page-bids fetch resolves, the SPA hook immediately writes
ts.adSlots/ts.bidsand callsts.adInit().adInit()looks up each slot element once and silently skips missing elements. Many SPA routers update history before the new route DOM has committed. - Why it matters: A fast edge response can run before the ad container exists, dropping server-side bids for that route with no retry and no beacon path.
- Suggested fix: Defer route application until the next render tick and/or retry missing slots for a bounded window (for example
requestAnimationFrameplusMutationObserver/timeout). Add a test where the page-bids response resolves before the slot element is inserted.
- Issue: After the page-bids fetch resolves, the SPA hook immediately writes
-
Build-time creative-opportunity validation only checks slot IDs —
crates/trusted-server-core/build.rs:131- Issue: The build script deserializes creative-opportunity slots as raw JSON and validates only
id, while the runtime schema requires fields likepage_patternsandformatsand denies unknown fields. - Why it matters: Bad slot config can pass
cargo buildand only fail later when the embedded settings are loaded at runtime/deploy. - Suggested fix: Validate
slot_rawagainst a build-time schema that mirrors the runtime required fields/types, or reuse the real runtime deserializer in build context.
- Issue: The build script deserializes creative-opportunity slots as raw JSON and validates only
P3 / Low
None.
CI / Existing Reviews
Current GitHub checks are passing (cargo fmt/test, clippy/Analyze, vitest, browser and integration tests, CodeQL, docs/TS formatting). Existing review threads show multiple prior blocking findings have been addressed; the findings above are based on current head behavior. Inline comments were attempted first, but GitHub rejected the batch as line-unresolvable, so findings are consolidated in this review body.
Address three review findings in the server-side ad-template flow:
- page-bids gate now requires Sec-Fetch-Site: same-origin and no longer
accepts same-site, which would admit sibling origins under the same
registrable domain that are not trusted to spend SSP quota. The
absent-metadata legacy fallback (X-TSJS-Page-Bids header) is unchanged.
- The Prebid requestBids shim no longer drops inline server-side bidder
params on a second call with the same ad unit. The first call prunes
server-side bidders from unit.bids, so a later call rebuilt bidderParams
as {}; it now retains the params captured on the first call when no new
server-side bidders are present.
- The SPA navigation hook defers applying bids until the new route's ad
containers exist (MutationObserver, bounded by a 2s timeout) so a fast
edge response cannot beat the DOM and silently drop server-side bids.
P1.2: handle_publisher_request forced Cache-Control: private and stripped surrogate headers on every text/html response, so a zero-slot or non-matching navigation lost shared cacheability even though no per-user ad data was injected. Gate that rewrite on should_run_ad_stack instead. First-visit identity responses are still protected: finalize_response now downgrades any Set-Cookie response to private and strips surrogate headers, since ec_finalize_response sets the EC cookie without a cache directive of its own. Returning-visitor, cookieless, non-ad HTML keeps its origin cache headers. P1.1: the slotRenderEnded win/billing beacon fired for any non-empty render whenever an APS bid existed for the slot (the !!hb_bidder fallback), over-reporting wins and billing for impressions won by other GAM demand. Require an hb_adid match instead, in both the bundle listener and the inline bootstrap. Slot targeting is still request state rather than proof of the winning line item, but this removes the unconditional false positive.
|
Thanks for the pass — addressed the five behavioral findings across two commits (065cea6, 5b225ae). Summary of how each was resolved: P1.1 — win/billing beacons could fire for non-TS winners (5b225ae) P1.2 — cache policy applied to all HTML even when the stack is no-op (5b225ae)
P2.1 — page-bids accepted same-site cross-origin callers (065cea6) P2.2 — repeated requestBids() dropped inline server-side bidder params (065cea6) P2.3 — SPA auctions could apply before the route's containers exist (065cea6) P2.4 — build-time validation only checks slot IDs CI green locally: cargo fmt/clippy, full workspace tests (1391 core + 45 adapter), and vitest (353). |
Merging main brought back the formats.rs make_bid helper, whose Bid literal predated the cache_id/cache_host/cache_path/ad_id fields this branch added to auction::types::Bid. Set them to None so the lib test target compiles. Pure test-helper fix; no change to /auction behavior.
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Automated review: Reviewed PR #680 at 0283b13 against main. CI is green, but I found blocking correctness issues in the server-side ad-template runtime: win/billing beacons can still fire without proof that the TS bid actually won, and SPA navigation can drop later-mounted slots on multi-slot routes. I also found a medium compatibility issue in the new content-type gates.
Summary of findings:
- P1: Win/billing beacons still use GPT slot targeting as winner proof (bundle and inline bootstrap).
- P1: SPA page-bids DOM wait resolves when any slot appears, so later-rendered slots are skipped permanently.
- P2: Case-sensitive Content-Type checks can bypass processing after dispatching auctions. The inline diff comment did not attach cleanly; the affected checks are
crates/trusted-server-core/src/publisher.rsaroundorigin_content_type.contains("text/html"),params.content_type.contains("text/html"), andis_processable_content_type. MIME types are case-insensitive, so normalize the header value (or parse MIME types case-insensitively) and add coverage forText/HTML; charset=utf-8.
CI / existing reviews: all current GitHub checks pass at this head. I reviewed the prior feedback and focused on issues still present after the latest fixes.
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Automated review:
Review Summary
Reviewed PR #680 at head 6a35abb against main, focusing on the server-side ad template auction/rendering path, GPT/Prebid refresh behavior, and recent follow-up fixes. CI is currently green. I found one blocking billing/rendering correctness issue in the TS render bridge and two medium follow-ups around refresh parity.
Findings
P0 / Blockers
None.
P1 / High
- Render bridge accepts Prebid requests from the wrong configured slot —
crates/js/lib/src/integrations/gpt/index.ts:675- Issue: The bridge only checks that
event.sourcebelongs to some configured slot iframe, then resolvesadIdagainst any entry inwindow.tsjs.bids. It does not require the source iframe's slot to match the slot whose bid supplied thathb_adid. - Why it matters: An iframe under slot A can ask for slot B's TS ad id, causing slot B's creative dimensions and bid data to be returned to slot A and firing slot B's win/billing beacons. In Trusted Server's auction flow this can mis-render creatives and over-report billable wins for the wrong placement.
- Suggested fix: Make the source check return the source slot id (or otherwise derive it from the iframe root), then require it to equal the slot id matched from
hb_adidbefore posting a Prebid Response or firing beacons. Add a two-slot Vitest regression where slot A's iframe requests slot B'shb_adidand assert no response, fetch, or beacon.
- Issue: The bridge only checks that
P2 / Medium
-
Refresh auctions omit configured client-side bidders —
crates/js/lib/src/integrations/prebid/index.ts:669- Issue:
installRefreshHandler()synthesizes refresh ad units with only thetrustedServerbidder. The requestBids shim preserves configuredclientSideBiddersonly when those bidder entries are already present on the ad unit, so native browser bidders are never included for refresh/scroll impressions. - Why it matters: Publishers that intentionally split demand between server-side bidders and client-side Prebid adapters lose that client-side demand on refreshes, even though the refresh handler is the path meant to run client-side auctions after initial render.
- Suggested fix: Reconstruct refresh ad units from matching
pbjs.adUnitsentries (preserving client-side bidder params), or keep a slot/zone-to-client-bidder config map and include those bidder entries in the synthetic refresh ad units. Add a test withclientSideBiddersconfigured and assert the refreshrequestBidscall includes both trustedServer and the native bidder.
- Issue:
-
Inline GPT bootstrap does not set the refresh-bypass sentinel —
crates/trusted-server-core/src/integrations/gpt_bootstrap.js:98- Issue: The TypeScript
adInit()setsts.adInitRefreshInProgressaround its internal GPT refresh so slim-Prebid's refresh wrapper passes that first server-side-targeted refresh through. The edge-injected bootstrap callsrefresh(slotsToRefresh)directly. - Why it matters: If the refresh wrapper is installed before the bootstrap path runs (for example under a different bundle ordering or future loader change), the initial TS refresh can be mistaken for a publisher refresh, clearing freshly applied targeting and starting a duplicate client auction.
- Suggested fix: Mirror the TypeScript implementation in the bootstrap with
ts.adInitRefreshInProgress = true; try { refresh(...) } finally { ts.adInitRefreshInProgress = false; }and add parity coverage.
- Issue: The TypeScript
P3 / Low
None.
CI / Existing Reviews
gh pr checks 680 reports all current checks passing (cargo fmt/test, clippy/analyze, vitest, browser/integration tests, docs/TS formatting, CodeQL). I also reviewed existing comments to avoid re-reporting prior issues that are already fixed; the source-slot mismatch appears distinct from the earlier general beacon over-fire findings.
Resolve three #680 review findings on the server-side ad runtime: - Render bridge now requires the requesting iframe's slot to own the resolved hb_adid before responding or firing win/billing beacons. Previously an iframe under slot A could request slot B's adId and receive slot B's creative while firing slot B's beacons. - Refresh ad units now include configured client-side bidders by merging matching pbjs.adUnits bid entries, so native Prebid demand is not dropped on refresh/scroll impressions. - Inline GPT bootstrap wraps its internal refresh with the adInitRefreshInProgress sentinel, mirroring the TS adInit so a pre-installed slim-Prebid refresh wrapper does not clear TS targeting. Add regression tests for the two-slot render-bridge mismatch and the client-side bidder refresh merge.
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Automated review: Reviewed PR #680 at 8fb30b3 against origin/main. CI is green and prior blocking review rounds appear largely addressed. I did not find blocking correctness/security issues that require changes, but I left three medium-priority inline comments around the inline GPT bootstrap, creative-opportunity config validation, and win/billing beacon delivery robustness.
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Automated review: I reviewed PR #680 at ceeae6f against origin/main, including CI and existing review feedback. CI is green, but I found blocking issues that should be addressed before merge.
Findings
P1 / High
-
/auctionbypasses the new server-side auction consent gate —crates/trusted-server-core/src/auction/endpoints.rs:148-189- Issue: the publisher-navigation and
/__ts/page-bidspaths now callconsent_allows_server_side_auction, which fails closed for GDPR or unknown jurisdictions unless effective TCF Purpose 1 is present.POST /auctionstill proceeds directly toorchestrator.run_auction()after only stripping EC IDs/EIDs. - Why it matters: this still dispatches PBS/APS calls and forwards request-derived signals such as UA/IP/geo (and, depending on Prebid consent-forwarding mode, cookies) for traffic the new gate says must not run a server-side auction. Since this PR documents
/auctionas the programmatic/server-side entry point, leaving it outside the same gate creates a privacy-policy inconsistency and an outbound partner-call path for no-Purpose-1/unknown-jurisdiction requests. - Suggested fix: apply
consent_allows_server_side_auction(&consent_context)before resolving EIDs or calling providers. If it returns false, return an empty/no-bid OpenRTB response without invokingrun_auction; add a regression test with GDPR/unknown consent lacking Purpose 1 that proves no provider is called.
- Issue: the publisher-navigation and
-
Build-time slot validation is incomplete, so broken config can ship — see inline comment on
crates/trusted-server-core/build.rs.
CI / Existing Reviews
All current PR checks pass (cargo fmt/test, clippy/analyze, vitest, browser/integration tests, CodeQL, docs/TS format). Existing review threads covered several related server-side ad-template concerns; I did not re-report those unless they remain present at the current head.
|
|
||
| /// No-op stub — full slot-shape validation runs at runtime against | ||
| /// the real creative opportunity types. | ||
| pub fn validate_runtime(&self) -> Result<(), String> { |
There was a problem hiding this comment.
Automated review: P1 / High — build-time slot validation is a stub, so broken env/TOML config can ship
build.rs deserializes creative opportunities into a stub type where compile_slots() and validate_runtime() are no-ops, and the later manual check only validates that each raw slot has a syntactically safe id. The real runtime validator rejects empty page_patterns, unmatchable patterns, empty formats, zero dimensions, and empty GAM unit paths, but those errors are not caught before writing target/trusted-server-out.toml.
Why it matters: an invalid trusted-server.toml or TRUSTED_SERVER__CREATIVE_OPPORTUNITIES__SLOT override can pass cargo check/CI and be embedded into the binary, then get_settings() reparses the embedded TOML with the real validator on requests and returns configuration errors at runtime. That turns a deployment/config mistake into a shipped service outage instead of a failed build.
Suggested fix: Either reuse the real creative-opportunity types in build.rs or duplicate the same semantic validation here before serializing the embedded config: non-empty valid page patterns, non-empty formats, positive dimensions, and non-empty resolved GAM unit path. Add a build/config round-trip test for an env-provided invalid slot such as formats: [] that must fail at build time.
There was a problem hiding this comment.
Fixed in 4d4fb1b (supersedes my earlier "deferred" reply above).
Extracted the slot validation into a shared creative_slot_build_check module, used by build.rs (via #[path]) and the crate test build (via #[cfg(test)] mod), so the rules now run under cargo test rather than living only in the build script. It mirrors CreativeOpportunitySlot::validate_runtime:
- non-empty id with the safe
[A-Za-z0-9_-]charset - at least one non-empty page pattern
- at least one format, each with positive width and height
- non-empty resolved GAM unit path (explicit override, else derived
/<gam_network_id>/<id>)
It runs against the merged config (base trusted-server.toml plus TRUSTED_SERVER__* env overrides) and before trusted-server-out.toml is serialized/written, so an invalid slot — including an env-injected formats: [] — fails the build and is never persisted to the embedded file. Verified manually for empty formats, zero dimensions, empty/blank page patterns, and blank GAM override; the same cases plus the well-formed and missing/invalid-id cases are covered by unit tests in the shared module.
Pattern compilability (the ** glob edge case) is still only enforced by the runtime validator — duplicating glob compilation in the build script would pull glob into build-deps; the non-empty-pattern check here covers the documented outage cases.
…r-side-ad-templates-impl Reconcile the server-side ad feature with main's migration of the HTTP type surface (fastly::Request/Response/Body -> http + EdgeBody + Platform traits). Key integrations: - handle_publisher_request: keep EC/consent/auction-dispatch/ad-slot logic, rebased onto PlatformBackendSpec + http_client().send and Response<EdgeBody>. - AuctionProvider: add async parse_response_with_context(PlatformResponse); port adserver_mock; make dispatch_auction/collect_dispatched_auction async to await the now-async request_bids/parse paths. - Streaming: keep main's deferred HandlerOutcome::Streaming, but drive it through stream_publisher_body_async so server-side auctions still run before </body>. - handle_page_bids and helpers ported from fastly APIs to http types. - Drop the BufferedProcessed route (HEAD streams all processable content). - Keep HEAD's apply_floor_prices semantics (undecoded None-price bids dropped). main's migration supersedes HEAD's pre-migration utility files (proxy, geo, permutive, prebid parse_response). Recovered three HEAD tests the auto-merge dropped (stream round-trip gzip/brotli, request_ec_uses_cookie_not_header).
Re-add publisher_request_uses_platform_http_client_with_http_types, dropped during the main merge because it called the pre-feature 4-arg handle_publisher_request. A run_publisher_proxy test helper supplies the no-auction EC/AuctionDispatch wiring so the test body stays a plain (settings, registry, services, req) proxy call.
The publisher-navigation and /__ts/page-bids paths fail closed for GDPR or unknown jurisdictions that lack effective TCF Purpose 1, but POST /auction proceeded straight to run_auction after only stripping EC IDs/EIDs — still dispatching PBS/APS calls and forwarding request-derived signals (UA/IP/geo, and cookies under some Prebid consent-forwarding modes) for traffic the gate says must not run a server-side auction. Apply consent_allows_server_side_auction before resolving EIDs or contacting providers; when it denies, return an empty no-bid OpenRTB response without invoking run_auction. Add a regression test that registers a panic-on-bid provider and proves a GDPR/unknown request lacking Purpose 1 returns no bids without contacting any provider. Route the orchestration-failure /auction tests through a non-regulated geo so they still exercise the provider path.
build.rs deserialized slots into a stub whose validate_runtime was a no-op and only checked slot-id syntax, so an invalid trusted-server.toml or TRUSTED_SERVER__CREATIVE_OPPORTUNITIES__SLOT override (empty page_patterns, empty formats, zero dimensions, empty resolved GAM unit path) passed CI and got embedded, then failed at request time as a configuration error. Extract the validation into creative_slot_build_check, shared by build.rs (via #[path]) and the crate test build (via #[cfg(test)] mod) so the rules run under cargo test. It mirrors CreativeOpportunitySlot::validate_runtime and runs against the merged config (base TOML plus TRUSTED_SERVER__* env overrides) before the config is serialized and embedded, so an invalid slot fails the build and is never persisted.
|
Re: review pullrequestreview-4497623498 — addressing the two P1 findings. P1 —
Regression test ( P1 — build-time slot validation is a stub. Fixed in 4d4fb1b — see the inline reply on Both verified locally: |
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Automated review: I reviewed PR #680 at 4d4fb1b against main. CI is currently green, and several prior review findings are addressed, but I found remaining blocking correctness/availability issues in the server-side ad-template auction paths.
Blocking body finding (call site is not on a current diff line, so not submitted inline):
- P1 / High — synchronous mediated auctions lose restored render/accounting fields —
crates/trusted-server-core/src/auction/orchestrator.rs:270- Issue:
run_parallel_mediation()still parses the mediator response throughparse_response(...). The currentadserver_mockmediator restoresnurl,burl,ad_id, and PBS cache fields from SSP responses only inparse_response_with_context(...), so the synchronous mediation path used byPOST /auctionand/__ts/page-bidscan return winning mediated bids withouthb_adid/ cache metadata. - Why it matters: mediated PBS cache bids can fail creative rendering and win/billing beacon firing even though the dispatched publisher collect path preserves those fields.
- Suggested fix: call
mediator.parse_response_with_context(platform_resp, response_time_ms, &mediator_context).awaithere, matching the dispatched collect path, and add a regression test that an adserver_mock-mediated bid preserves cache/nurl fields throughrun_auction().
- Issue:
Other findings are inline.
| // later defineSlot on the inner div doesn't conflict. | ||
| const containerEl = document.getElementById(`${actualDivId}-container`); | ||
| const slotDivId = containerEl?.id ?? actualDivId; | ||
| const defined = g.defineSlot?.(slot.gam_unit_path, slot.formats, slotDivId); |
There was a problem hiding this comment.
Automated review: P1 / High — TS-owned GPT slots are defined but never displayed before refresh
In the fallback path where Trusted Server defines a slot because the publisher has not already done so, the code calls defineSlot(...).addService(...) and later pubads().refresh(slotsToRefresh), but it never calls googletag.display(slotDivId) for that newly-defined slot. GPT requires a display call to register/render a slot; undisplayed slots produce the documented defineSlot was called without a matching display call failure mode and can miss impressions. Reused publisher-owned slots are fine because the publisher likely displayed them; TS-owned first-impression slots can no-op in real GPT. The same issue exists in the inline bootstrap (crates/trusted-server-core/src/integrations/gpt_bootstrap.js).
Suggested fix: track TS-owned slot element IDs separately, call googletag.display(slotDivId) once after targeting/services are ready, and do not also include those same TS-owned slots in the immediate refresh() call. Keep refresh() for reused publisher-owned slots. Mirror the bootstrap behavior and add Vitest/bootstrap coverage.
| } | ||
|
|
||
| // At least one non-empty page pattern. | ||
| let has_valid_pattern = slot |
There was a problem hiding this comment.
Automated review: P1 / High — build-time slot validation misses invalid glob patterns
The build-time validator only checks for a non-empty page_patterns string. Runtime preparation later compiles patterns and rejects a slot when no pattern compiles, so a private/env config such as page_patterns = ["["] can pass the release build and then make the deployed service fail settings load on startup/request. That defeats the purpose of this build-time guard.
Suggested fix: have validate_creative_slot compile patterns with the same glob::Pattern::new + ** normalization contract used by runtime compile_patterns(), and add a build-check test for an invalid pattern that remains invalid after normalization.
| .headers() | ||
| .get(header::CACHE_CONTROL) | ||
| .and_then(|v| v.to_str().ok()) | ||
| .is_some_and(|v| v.contains("private") || v.contains("no-store")); |
There was a problem hiding this comment.
Automated review: P2 / Medium — Cache-Control privacy checks are case-sensitive
HTTP cache directives are case-insensitive, but these checks look for lowercase private / no-store substrings. Cache-Control: No-Store on a Set-Cookie response is treated as cacheable and overwritten with the weaker private, max-age=0; similarly, Cache-Control: Private will not protect the response from operator response_headers replacing cache/surrogate headers below.
Suggested fix: parse directives case-insensitively (at minimum lowercase the header value before matching), and add mixed-case No-Store / Private tests.
| # drains in <50 ms but the auction runs to the limit. 500 ms is the recommended | ||
| # default; raise only if your SSPs need more headroom and your analytics confirm | ||
| # the DCL slip is acceptable. | ||
| auction_timeout_ms = 1500 # override via TRUSTED_SERVER__CREATIVE_OPPORTUNITIES__AUCTION_TIMEOUT_MS |
There was a problem hiding this comment.
Automated review: P2 / Medium — checked-in creative timeout contradicts its latency guidance
The comment recommends a 500ms default because this value bounds DOMContentLoaded/window.load slip, but the checked-in value is 1500ms. Since private configs can enable slots while inheriting this default, the first production rollout can impose a 1.5s close-body hold on cache-hit pages.
Suggested fix: set the sample/default to 500, or update the comment/docs if 1500ms is intentionally the recommended operational default.
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary (second review pass)
Re-review of PR #680 at head 4d4fb1b2. The prior review's findings are substantially resolved with tests — beacon over-firing, runtime slot-id validation, the SPA-hook coverage gap, the leaked global in the JS tests, page-bids path normalization, and the redundant constructors are all fixed. Two new, well-isolated bugs remain (one-line/one-function fixes), plus parity and cleanup items. Inline comments carry most findings; cross-cutting items that don't pin to a changed line are below.
Blocking
🔧 wrench
price_buckettruncates instead of rounding —price_bucket.rs:24(inline). Under-buckets common CPMs (0.29→"0.28",1.15→"1.14", …) that becomehb_pbtargeting keys.- Sync mediation path drops bid metadata (nurl/burl/ad_id) —
auction/orchestrator.rs:269.run_auction → run_parallel_mediationparses the mediator response with the context-lessparse_response, while the async collect path correctly usesparse_response_with_context(:1015).adserver_mock::parse_responserebuilds an emptyBidIndex, so nurl/burl/ad_id are dropped — and that function's own comment falsely claims "the orchestrator always callsparse_response_with_context." Reachable via the POST/auctionendpoint. Fix: change the one call site toparse_response_with_context(platform_resp, response_time_ms, &mediator_context)(the context is already constructed in scope).
Non-blocking (not pinnable to a single changed line)
🤔 thinking
- Dispatched auction silently dropped on
Buffered/PassThroughroutes —publisher.rs:1342-1398.should_run_auctionis decided from request signals before the origin content-type/status/encoding is known. A navigation that already dispatched SSP bid requests but then routes toBufferedUnmodified(non-2xx, unsupported encoding e.g.zstd, empty host) orPassThrough(2xx non-HTML) drops theDispatchedAuctionwithoutcollect_dispatched_auction— the in-flight partner requests are wasted and the pending handles left uncollected. Not a crash, but wasted SSP quota and lost bids. At minimumlog::warn!whendispatched_auction.is_some()on those arms (ideally await-and-discard the result).
🌱 seedling
- Sync mediator timeout not capped at
timeout_ms—auction/orchestrator.rs:243.run_parallel_mediationgives the mediator the full remaining auction budget, while the collect path tightens it toremaining.min(mediator.timeout_ms()). Not a correctness bug (remaining budget is still a valid bound), but the two paths differ; apply.min(mediator.timeout_ms())for symmetry. - Coverage gap: env-injected slot-id rejection untested — there is a TOML-path test (
settings_rejects_invalid_creative_opportunity_slot_id) but none assertingTRUSTED_SERVER__CREATIVE_OPPORTUNITIES__SLOT='[{"id":"bad id",…}]'is rejected viafrom_toml_and_env/ build-timevalidate_creative_slot. Adding one locks in the regression that the runtime-validation fix addressed.
📝 note
/20**still used as the primary doc example for a path glob —creative_opportunities.rs(~:161). That string is an invalid glob that only "works" via the**→*normalization fallback; using it as the canonical example invites copy-paste of broken config. Prefer a valid example (e.g."/2024/*"), keeping the normalization note as the edge-case caveat.
Prior-findings resolution (round 1)
Resolved: beacon over-fire/double-fire; validate_slot_id dead at runtime; JS test addEventListener leak + duplication; installSpaAuctionHook coverage; Prebid stored-request fan-out (documented intentional); page-bids path normalization; Cache-Control intent; collect-loop deadline check; redundant dense()/banner() constructors; main.rs cookie-path cache guard; POST /auction consent gate.
Still open: cookies.rs::parse_ts_eids_cookie dead code (♻️, inline); APS provider Mutex state (🤔, inline — adserver_mock was migrated, APS was not).
CI Status
- cargo fmt: PASS
- cargo clippy: PASS
- cargo test: PASS
- vitest (JS): PASS
- CodeQL / integration / browser-integration: PASS
5 participants
Summary
[creative_opportunities]intrusted-server.toml. Matching slots are selected from the incoming document URL at the edge, andwindow.tsjs.adSlotsis injected at<head>open so initial GPT setup does not need a separate slot-discovery request.window.tsjs.bidsbefore</body>.window.tsjs.adInitGPT runtime path. It readswindow.tsjs.adSlotsandwindow.tsjs.bidssynchronously, defines or reuses GPT slots, applies slot-level andhb_*targeting, setsts_initial=1, refreshes the initial slots, and firesnurl/burlonly afterslotRenderEndedconfirms the TS bid won viahb_adid.[integrations.prebid].suppress_nurl_bidders, while retaining the deployment-wide[integrations.prebid].suppress_nurlcompatibility switch.window.tsjs.adSlots/ GPT metadata instead of placeholder refresh sizes.ts-eidscookie written by TSJS, gates EIDs by consent, and forwards them as OpenRTBuser.ext.eidsto PBS.ClientInfointo the server-side PBS request so bidders see the browser client context rather than the Fastly edge context.GET /__ts/page-bidshook, which updateswindow.tsjs.adSlots/window.tsjs.bidsand re-runswindow.tsjs.adInit()afterpushState,replaceState, orpopstatenavigations.What the server-side auction sends to PBS
user.idts-eccookie or generated EC identityuser.consent/user.ext.consentuser.ext.eidsts-eidscookie plus EC identity resolutionuser.ext.ec_freshdevice.uaUser-Agentheaderdevice.ipClientInfo.client_ipdevice.geodevice.dnttrueif setDNT: 1headerdevice.languageAccept-Languageheadersite.domain/site.pagesite.refRefererheaderregs.gdpr/regs.us_privacy/regs.gppimp.*[creative_opportunities]slot templates and Prebid configtmax[creative_opportunities].auction_timeout_ms, falling back to[auction].timeout_msChanges
trusted-server.toml[creative_opportunities]slot templates and documents Prebid nurl suppression knobs..env.example/ docsSUPPRESS_NURL_BIDDERS.crates/trusted-server-core/src/creative_opportunities.rscrates/trusted-server-core/src/settings.rs/build.rstrusted-server.toml.crates/trusted-server-core/src/publisher.rswindow.tsjs.adSlotsandwindow.tsjs.bids, applies cache-control safeguards, handles page-bids responses, and forwards client context.crates/trusted-server-core/src/html_processor.rstsjs.bidsinjection.crates/trusted-server-core/src/auction/*crates/trusted-server-core/src/integrations/prebid.rsnurl/burlpropagation, and per-bidder suppression.crates/trusted-server-core/src/integrations/aps.rscrates/trusted-server-core/src/integrations/adserver_mock.rscrates/trusted-server-core/src/integrations/gpt.rs/gpt_bootstrap.jswindow.tsjs.adInitbootstrap path.crates/js/lib/src/integrations/gpt/index.tswindow.tsjs.adSlots,window.tsjs.bids, render-confirmed beacon firing, SPA updates, slim-Prebid loading, and Prebid creative rendering bridge.crates/js/lib/src/integrations/prebid/index.tscrates/trusted-server-adapter-fastly/src/main.rsCloses
Closes #677
Closes #697
Closes #698
Closes #699
Closes #700
Closes #702
Test plan
Automated
cargo test --workspacecargo clippy --workspace --all-targets --all-features -- -D warningscargo fmt --all -- --checkgit diff --checkcd crates/js/lib && node build-all.mjscd crates/js/lib && npm run lintcd crates/js/lib && npm run formatcd docs && npm run formatcd crates/js/lib && npx vitest runcurrently fails before test discovery in this workspace withERR_REQUIRE_ESMfromhtml-encoding-snifferrequiring@exodus/bytes/encoding-lite.js; no local JS test assertions execute under that failure mode.Manual end-to-end (browser DevTools console)
The steps below build on each other. Use a URL whose path matches one of the configured
[[creative_opportunities.slot]]page_patterns.Step 1 - Verify slot config is injected at
<head>openExpected: an array of slot objects. Each entry has
id,gam_unit_path,div_id,formats, andtargeting. Note thediv_idvalue from one matching slot for step 3.Step 2 - Verify server-side auction results are injected before
</body>Expected: an object keyed by slot ID. Winning slots include
hb_bidder,hb_pb, and, for Prebid cache-backed bids,hb_adid/ cache fields.Step 3 - Verify
window.tsjs.adInitwired GPT targetingReplace
SLOT_DIV_IDwith thediv_idfrom step 1.Expected: the slot has the configured GAM unit path and targeting includes
hb_pb,hb_bidder, any slot-level keys such aspos/zone, andts_initial: ["1"].Step 4 - Verify slot matching is page-pattern-aware
Navigate to a different configured path, for example
/when homepage slots are configured, and repeat step 2.Expected:
window.tsjs.bidsis keyed by the slots matching that page, not by slots from the previous page type.Step 5 - Confirm no duplicate bids injection
View page source and search for
.bids=JSON.parse.Expected: exactly one
window.tsjs.bidsassignment before</body>on pages where an auction ran.Pending (GAM line items required)
Creative delivery requires standard GAM line items targeting
hb_pb,hb_bidder, and related Prebid keys. That setup is outside this PR.Checklist
unwrap()in production codelogmacros instead ofprintln!